feat: Automated test suite — unit, integration, component#870
Open
tuliomir wants to merge 10 commits into
Open
feat: Automated test suite — unit, integration, component#870tuliomir wants to merge 10 commits into
tuliomir wants to merge 10 commits into
Conversation
@testing-library/jest-dom@5.17 — DOM-aware Jest matchers (jest-dom@5.x matches Jest 27 from react-scripts@5.0.1). redux-saga-test-plan@4.0.6 — expectSaga + provide() for the Layer 2 saga integration tests. Refs RFC 0001 (auto-qa) Reference-level § Test dependencies.
Establish the test infrastructure described in RFC 0001
Reference-level § Helpers and § Centralized mocks. The shape
maps the mobile RFC to CRA conventions: helpers live in
src/test-utils/ (outside __tests__/ so CRA's testMatch
doesn't pick them up), and centralized package mocks live in
src/__mocks__/ next to CRA's roots.
Mocks (src/__mocks__/):
- @hathor/wallet-lib — dissolves the import chain that
fails Jest's default Babel transform (wallet-lib pulls in
ESM-only axios). Exposes the constants, classes, and
utilities the wallet reads at module-load time.
- @hathor/hathor-rpc-handler — same import-chain concern.
- @reown/walletkit, @walletconnect/{core,utils} — no-op
stubs; the real packages open WebSockets at import time.
- @sentry/electron — no-op telemetry.
- @ledgerhq/hw-transport-node-hid — scripted USB transport;
the file comment points contributors at the canonical
hathor-ledger-app repo for APDU contract divergences.
- unleash-proxy-client — fixed no-flags state, no polling.
setupTests.js: CRA-conventional global setup; activates the
mocks above via jest.mock() and pulls in jest-dom matchers.
Helpers (src/test-utils/):
- createTestStore — Redux store w/ saga+thunk middlewares
- renderWithProviders — RTL render wrapped in Provider+Router
- mockNavigation — react-router-dom v6 spy set
- getInitialState — root reducer's @@init state
- ledgerTransportMock — fluent scripted APDU responses
- index.js — barrel re-exports for convenience
State of the existing 9 tests after this commit:
- 5 suites now pass (PinPasswordWrapper, networkTokens-
Persistence, SelectTokenType, WalletAddress, CustomTokens).
- 4 suites still fail with pre-existing test-vs-production
drift unmasked by re-enabling CI (ModalPin, atomicSwap,
ModalSendTx, CreateToken). Addressed in the next commit.
Refs RFC 0001 (auto-qa) § Centralized mocks, § Helpers.
Re-enabling Jest in CI surfaced four pre-existing test suites
that have been silent since the npm test step was commented
out in main.yml. None is a regression introduced by this PR;
each was already broken on master. Triaging here so PR 1's
test infrastructure ships with a green baseline.
Resolved:
- ModalPin "rendering tests" — green w/ centralized mocks
- WalletAddress, CustomTokens, SelectTokenType, ModalSendTx
"rendering tests" — green w/ centralized mocks
Skipped with FIXME breadcrumbs (deferred to feature-area PRs):
- ModalPin "pin validation" — passes a `wallet` prop the
component no longer honours (uses getGlobalWallet()).
- ModalSendTx "tx handling" — reaches for a nested
ErrorMessages.ErrorMessages shape and deeper Send-
Transaction behaviour the smoke mock doesn't provide.
- atomicSwap "calculateExhibitionData" — constructs
PartialTxProposal instances directly with internal-state
expectations beyond what the smoke mock reproduces.
- CreateToken "Token Version Handling" — getByText collides
on text that appears in both the heading and the submit
button; needs scoping to getByRole.
The wallet-lib mock was filled in further to support the
suites that now pass: DEFAULT_NATIVE_TOKEN_CONFIG, TokenVersion,
WalletType, OutputType, NanoContractActionType, MemoryStore /
Storage, PartialTxProposal (as class with static factory),
PartialTx, ErrorMessages, Send/Transaction/Fee, dateUtils,
scriptsUtils, bufferUtils, bigIntUtils, cryptoUtils — all the
shapes the existing test surface reads at module-load time.
Baseline after this commit: 7 suites pass, 2 suites skipped,
34 tests pass, 18 tests skipped, 0 failures.
Refs RFC 0001 (auto-qa) § Drawbacks #4 — "9 pre-existing tests
have no documented contracts… reviewers will see them in the
diff under their new paths."
Three reference tests, each demonstrating one canonical pattern
that feature-area PRs will copy when adding more Layer-1 tests:
L1·util (src/__tests__/utils/tokens.test.js)
Simplest Jest pattern — pure function, no mocks, no state.
Tests tokens.getNFTFee() (returns 1n).
L1·selector (src/__tests__/utils/selectors.test.js)
Function consuming Redux state — shows how to build a test
state object via getInitialState() and pass it in.
Tests selectIsOnline, selectSelectedToken, selectTokenByUid.
L1·reducer (src/__tests__/reducers/reducer.wallet.test.js)
Three-contract pattern — initial-state shape, action-type
literals, behavior — each pinned independently as a safety
net for the eventual RTK-slices migration.
New production code:
- src/utils/selectors.js (10 lines, 3 selectors). Reference
selectors module; feature-area PRs add their extractions
here as they refactor connected components.
Test infrastructure:
- setupTests.js gains a mock for src/store/index.js. The chain
reducers/index → sagas/tokens → sagas/helpers → utils/tokens
→ store/index → reducers/index is benign at app startup (entry
order avoids re-entry) but cyclic from a test entry. The stub
breaks the cycle; tests use createTestStore for real stores.
Refs RFC 0001 (auto-qa) § PR 1 smoke set rows L1·util / L1·selector
/ L1·reducer.
Two reference tests covering the two saga shapes feature-area
PRs will keep adding to:
L2·saga (src/__tests__/sagas/wallet.test.js)
expectSaga + provide() — inject a mocked effect at the saga
boundary; assert on return value. Smallest saga in wallet.js
(`isAtomicSwapEnabled`) chosen so the pattern is unobscured
by saga-internal complexity. Future feature-area PRs cover
the larger sagas (startWallet, send-tx, Nano Contract
register) following this same shape.
L2·saga reset (src/__tests__/sagas/modal.test.js)
Demonstrates the *ForTesting module-state-reset pattern for
sagas that keep state outside the Redux store. modalSaga
holds a `modalContext` registration in module scope; tests
must call clearModalContextForTesting() in beforeEach to
avoid one test's setModalContext leaking forward to the
next.
Production code change (1 line + JSDoc):
- src/sagas/modal.js exports clearModalContextForTesting().
Test-only by convention (the suffix marks it). RFC 0001
explicitly describes this pattern in § Production code
changes expected.
Refs RFC 0001 (auto-qa) § PR 1 smoke set rows L2·saga / L2·saga reset.
Three reference tests, one per component test technique that
feature-area PRs will keep reaching for:
L3·happy (src/__tests__/screens/Welcome.test.jsx)
Render via renderWithProviders, assert on baseline UI text +
button + form input state. The smallest possible component
test — no interaction, no state mutation. Future feature-
area PRs add preloadedState to drive per-screen variants.
L3·error (src/__tests__/screens/SoftwareWalletWarning.test.jsx)
Assert on a non-happy-path UI state. Clicking Continue
without checking the agreement box triggers the bootstrap
`was-validated` class on the form — the canonical pattern
for screens that validate before submit. Future feature-
area PRs adapt this for error modals and inline banners.
L3·navigation (src/__tests__/screens/WalletType.test.jsx)
Assert that a user action triggers a navigation. Uses the
hoist-safe `const mockNavigate = jest.fn()` + jest.mock
pattern so future PRs that need similar assertions can copy
the shape. WalletType chosen over the RFC's indicative
Settings.test.jsx because Settings's render path needs
GlobalModalContext + several useSelector slices set up;
WalletType is the simplest screen that exercises two
distinct navigations without dragging in additional setup.
After this commit the suite reports 15 passing suites, 2
skipped, 75 tests pass, 18 skipped, 0 failures.
Refs RFC 0001 (auto-qa) § PR 1 smoke set rows L3·happy /
L3·error / L3·navigation.
react-scripts@5.0.1 + React 18 are now both in deps; the incompatibility cited in PRs #416 / #449 / #567 is gone, and the existing 9 tests have been re-greened (with two suites explicitly skipped under FIXME breadcrumbs). Step shape mirrors what was previously commented out, plus --watchAll=false so the test runner exits cleanly on CI: - Unit + integration + component tests - Upload coverage (codecov) Refs RFC 0001 (auto-qa) Reference-level § Reactivating Jest in CI.
Four files that together act as the documentation backbone
for every future test PR in this repo. Without them the same
review nits ("don't redeclare the central mock", "use
*ForTesting between tests") would recur on every feature-
area PR.
AGENTS.md (48 lines)
Cross-tool entry point. Brief authoritative rules pointing
to the deeper docs. Targeted at Claude Code, Gemini CLI,
Codex, OpenAI Copilot, and any future agent that reads a
conventional top-level AGENTS.md.
CLAUDE.md (74 lines)
Claude-Code-specific pointer. Same rule set as AGENTS.md
plus a note on how to load the writing-tests skill and a
short "things NOT to do" list. Kept tight on purpose so
every line is load-bearing.
.claude/skills/writing-tests/SKILL.md (275 lines)
Auto-loads when an agent touches a file under
src/__tests__/ or src/__mocks__/. Pyramid layout, the
reference-smoke / feature-area distinction, indexed table
of the eight reference smoke tests, helper APIs,
centralized-mock conventions, the three-contract reducer
pattern, the Ledger contract-mirror boundary, saga test
patterns, what NOT to test, and a diagnostic checklist for
the common failure modes.
docs/testing-guide.md (348 lines)
Long-form human reference. Top-to-bottom readable for a
new contributor who has never touched the test surface;
covers the same ground as the skill but with prose
explanation rather than terse rule list. Includes the
list of pre-existing tests skipped in PR 1 with their
FIXME root causes so the next feature-area PR knows where
to start.
Refs RFC 0001 (auto-qa) Reference-level § AI agent /
contributor documentation (deliberately a first-class
deliverable of PR 1, not a Future Possibility).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds comprehensive testing infrastructure: docs, centralized Jest mocks, test utilities, reference tests (reducers, sagas, components, utils), small production test seams, dev deps, and CI steps to run tests and upload coverage. ChangesTesting Infrastructure & Reference Test Suite (RFC-0001)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/__mocks__/unleash-proxy-client.js (1)
14-19: 💤 Low valueConsider documenting or removing unused handlers storage.
The
handlersobject is initialized and populated byon(), but the stored handlers are never invoked. Since the mock comment emphasizes "static no-flags-enabled state", this is likely intentional—tests shouldn't rely on event emission. However, the storage pattern might confuse future maintainers.Consider either:
- Adding a comment explaining that handlers are stored for API compatibility but intentionally not invoked.
- Removing the
handlersproperty entirely if no tests rely on it.Option 1: Document the intentional no-op
class UnleashClient { constructor() { + // Handlers stored for API compatibility but never invoked. + // Mock returns static state; tests should not rely on events. this.handlers = {}; }Option 2: Remove unused storage
class UnleashClient { - constructor() { - this.handlers = {}; - } on(event, handler) { - this.handlers[event] = handler; + // No-op: handlers not stored or invoked in this mock. return this; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__mocks__/unleash-proxy-client.js` around lines 14 - 19, The handlers object is stored by the mock (this.handlers) and populated via the on(event, handler) method but never invoked, which confuses maintainers; either remove this.handlers and make on(event, handler) a simple no-op that returns this, or keep the storage and add a clear inline comment in the constructor and above on() stating that handlers are stored only for API compatibility and intentionally not emitted in the static no-flags-enabled mock (refer to this.handlers and on(event, handler) to locate the code).src/test-utils/renderWithProviders.js (1)
42-44: 💤 Low valueConsider simplifying the store selection logic.
The current destructuring pattern works but is more complex than needed:
♻️ Optional simplification
- const { store: testStore } = store - ? { store } - : createTestStore(preloadedState); + const testStore = store ?? createTestStore(preloadedState).store;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test-utils/renderWithProviders.js` around lines 42 - 44, The destructuring used to obtain testStore is unnecessarily convoluted; replace the pattern in renderWithProviders so it directly picks the provided store or the created one, e.g., set testStore using the provided store or the store returned from createTestStore: assign testStore = store ?? createTestStore(preloadedState).store (refer to the variables store and testStore and the function createTestStore and the preloadedState parameter to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/writing-tests/SKILL.md:
- Around line 16-27: The Markdown code fence containing the directory tree is
missing a language identifier; update the opening fence from ``` to ```text for
the directory-tree block (the fenced block that starts with "__tests__/" and
lists utils/reducers/sagas/screens/components and the src/* lines) so the linter
MD040 is satisfied and the snippet is treated as plain text.
In @.github/workflows/main.yml:
- Around line 52-54: The workflow step named "Upload coverage" is pinned to an
old commit for codecov/codecov-action (v3.1.4); update the uses reference in
that step to the current stable major (for example
codecov/codecov-action@v6.0.0) or to the commit hash corresponding to the v6.0.0
release so you pick up the latest fixes and hardening while keeping the action
pinned; change the uses line in the "Upload coverage" step accordingly and run a
workflow test to confirm compatibility with the new action.
In `@docs/testing-guide.md`:
- Around line 34-48: The two fenced code blocks (the ASCII test pyramid diagram
and the src/ tree) in docs/testing-guide.md are missing language identifiers
which triggers MD040; update both opening fences from ``` to ```text so the
blocks are explicitly marked as plain text (apply the same change to the other
occurrence around lines 57-80); ensure only the opening fence is changed to
include the language identifier for the ASCII diagram and the src/ tree blocks.
In `@package.json`:
- Line 109: Update the dependency entry for `@testing-library/jest-dom` in
package.json from "5.17.0" to the latest stable release (e.g., "6.9.1"), run
npm/yarn install, then run the test suite and fix any failing assertions caused
by the major-version API changes; specifically review usages of jest-dom
matchers in your tests (imports and matcher names) and update code to the v6 API
where required.
In `@src/__mocks__/`@ledgerhq/hw-transport-node-hid.js:
- Line 43: The mock for TransportNodeHid.listen currently is just jest.fn() and
thus returns undefined, breaking callers that expect a subscription handle with
an unsubscribe() method; update the mock for listen to return an object with an
unsubscribe jest.fn() (i.e., change the mock export where listen is defined so
that listen: jest.fn(() => ({ unsubscribe: jest.fn() })) ) so tests and cleanup
code that call the returnedHandle.unsubscribe() won't fail; ensure the symbol
referenced is listen in the `@ledgerhq/hw-transport-node-hid` mock and that the
returned object contains unsubscribe().
In `@src/__tests__/sagas/modal.test.js`:
- Around line 36-51: The test currently creates and restores a console.error spy
inside the test body which can leak if the promise rejects; move the spy
lifecycle to a beforeEach/afterEach pair: create the spy (jest.spyOn(console,
'error').mockImplementation(...)) in beforeEach and call errorSpy.mockRestore()
in afterEach, leaving the individual test (which dispatches
types.SHOW_GLOBAL_MODAL to modalSaga) to only assert behavior; reference the
existing errorSpy variable name and the modalSaga/types.SHOW_GLOBAL_MODAL usage
so you place the setup/teardown in the same test scope.
In `@src/setupTests.js`:
- Line 57: The mocked Redux store's subscribe currently returns undefined;
update the mock for subscribe (the subscribe mock in src/setupTests.js) to
return an unsubscribe function so callers can call it safely—change the
jest.fn() for subscribe to a factory that returns a no-op function (e.g.,
jest.fn(() => () => {})) so it adheres to the Redux subscribe contract and
prevents crashes when callers invoke the returned unsubscribe handle.
In `@src/test-utils/createTestStore.js`:
- Around line 37-38: The Redux store configuration is adding redux-thunk
redundantly because getDefaultMiddleware() already includes thunk; remove the
explicit thunk addition: update createTestStore.js where middleware is built by
getDefaultMiddleware(...).concat(sagaMiddleware, thunk) to drop the thunk so it
only concatenates sagaMiddleware, and likewise update the production store setup
where the middlewares array currently contains [sagaMiddleware, thunk] (and
where getDefaultMiddleware(...).concat(middlewares) is used) to remove thunk
from that array; keep sagaMiddleware and ensure only sagaMiddleware is appended
to getDefaultMiddleware so thunk remains provided by RTK's getDefaultMiddleware.
---
Nitpick comments:
In `@src/__mocks__/unleash-proxy-client.js`:
- Around line 14-19: The handlers object is stored by the mock (this.handlers)
and populated via the on(event, handler) method but never invoked, which
confuses maintainers; either remove this.handlers and make on(event, handler) a
simple no-op that returns this, or keep the storage and add a clear inline
comment in the constructor and above on() stating that handlers are stored only
for API compatibility and intentionally not emitted in the static
no-flags-enabled mock (refer to this.handlers and on(event, handler) to locate
the code).
In `@src/test-utils/renderWithProviders.js`:
- Around line 42-44: The destructuring used to obtain testStore is unnecessarily
convoluted; replace the pattern in renderWithProviders so it directly picks the
provided store or the created one, e.g., set testStore using the provided store
or the store returned from createTestStore: assign testStore = store ??
createTestStore(preloadedState).store (refer to the variables store and
testStore and the function createTestStore and the preloadedState parameter to
locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c455a03a-8847-45bc-84dd-13e7fae739bf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.claude/skills/writing-tests/SKILL.md.github/workflows/main.ymlAGENTS.mdCLAUDE.mddocs/testing-guide.mdpackage.jsonsrc/__mocks__/@hathor/hathor-rpc-handler.jssrc/__mocks__/@hathor/wallet-lib.jssrc/__mocks__/@ledgerhq/hw-transport-node-hid.jssrc/__mocks__/@reown/walletkit.jssrc/__mocks__/@sentry/electron.jssrc/__mocks__/@walletconnect/core.jssrc/__mocks__/@walletconnect/utils.jssrc/__mocks__/unleash-proxy-client.jssrc/__tests__/components/ModalPin.test.jssrc/__tests__/components/ModalSendTx.test.jssrc/__tests__/reducers/reducer.wallet.test.jssrc/__tests__/sagas/modal.test.jssrc/__tests__/sagas/wallet.test.jssrc/__tests__/screens/CreateToken.test.jssrc/__tests__/screens/SoftwareWalletWarning.test.jsxsrc/__tests__/screens/WalletType.test.jsxsrc/__tests__/screens/Welcome.test.jsxsrc/__tests__/utils/atomicSwap.test.jssrc/__tests__/utils/selectors.test.jssrc/__tests__/utils/tokens.test.jssrc/sagas/modal.jssrc/setupTests.jssrc/test-utils/createTestStore.jssrc/test-utils/getInitialState.jssrc/test-utils/index.jssrc/test-utils/ledgerTransportMock.jssrc/test-utils/mockNavigation.jssrc/test-utils/renderWithProviders.jssrc/utils/selectors.js
Comment on lines
+16
to
+27
| ``` | ||
| __tests__/ | ||
| ├── utils/ # L1 — pure functions, selectors | ||
| ├── reducers/ # L1 — reducer three-contracts tests | ||
| ├── sagas/ # L2 — saga integration (expectSaga + provide()) | ||
| ├── screens/ # L3 — component render + interaction | ||
| └── components/ # L3 — non-screen reusable component tests | ||
|
|
||
| src/test-utils/ # helpers used by tests (renderWithProviders, etc.) | ||
| src/__mocks__/ # manual mocks for npm packages | ||
| src/setupTests.js # CRA-conventional global setup; activates the mocks | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the directory-tree code fence.
This fence is missing a language tag (MD040). Use text to keep markdown lint clean.
Proposed fix
-```
+```text
__tests__/
├── utils/ # L1 — pure functions, selectors
├── reducers/ # L1 — reducer three-contracts tests
├── sagas/ # L2 — saga integration (expectSaga + provide())
├── screens/ # L3 — component render + interaction
└── components/ # L3 — non-screen reusable component tests
src/test-utils/ # helpers used by tests (renderWithProviders, etc.)
src/__mocks__/ # manual mocks for npm packages
src/setupTests.js # CRA-conventional global setup; activates the mocks</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/writing-tests/SKILL.md around lines 16 - 27, The Markdown
code fence containing the directory tree is missing a language identifier;
update the opening fence from totext for the directory-tree block (the
fenced block that starts with "tests/" and lists
utils/reducers/sagas/screens/components and the src/* lines) so the linter MD040
is satisfied and the snippet is treated as plain text.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+52
to
+54
| - name: Upload coverage | ||
| # https://github.com/codecov/codecov-action/releases/tag/v3.1.4 | ||
| uses: codecov/codecov-action@eaaf4bedf32dbdc6b720b63067d99c4d77d6047d |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of codecov/codecov-action GitHub Action and are there any security advisories for v3.1.4?
💡 Result:
Latest stable version of codecov/codecov-action (as of 2026-05-22): v6.0.0 [1][2]. Security advisories for v3.1.4: I did not find any GitHub Security Advisory (GHSA) entry or other official security advisory page specifically mentioning v3.1.4. The only v3.1.4 material I found is the changelog describing non-security fixes (typos, CLI option names, and dev dependency bumps) rather than a security issue [3][4]. If you want to be extra sure, search the repository’s “Security” → “Advisories” (or GitHub’s GHSA listing) for entries that mention “3.1.4” or “v3.1.4”; based on the sources above, no such advisory surfaced in my research.
Citations:
- 1: https://github.com/codecov/codecov-action/tree/refs/heads/main
- 2: https://github.com/codecov/codecov-action
- 3: https://github.com/codecov/codecov-action/blob/v3.1.5/CHANGELOG.md
- 4: codecov/codecov-action@v3.1.3...v3.1.4
Update Codecov action pin (current major is v6.0.0)
codecov/codecov-action is pinned to eaaf4bedf32dbdc6b720b63067d99c4d77d6047d (v3.1.4 from 2023). The latest stable release is v6.0.0. I didn’t find a GHSA/security advisory specifically for v3.1.4, but this pin is several major versions behind—upgrade (still pinned to a commit) to pick up maintenance/bug fixes and any security hardening from newer releases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/main.yml around lines 52 - 54, The workflow step named
"Upload coverage" is pinned to an old commit for codecov/codecov-action
(v3.1.4); update the uses reference in that step to the current stable major
(for example codecov/codecov-action@v6.0.0) or to the commit hash corresponding
to the v6.0.0 release so you pick up the latest fixes and hardening while
keeping the action pinned; change the uses line in the "Upload coverage" step
accordingly and run a workflow test to confirm compatibility with the new
action.
Comment on lines
+34
to
+48
| ``` | ||
| ┌─────────┐ | ||
| │ E2E │ ← Playwright + Electron (PR 2) | ||
| │ (few) │ | ||
| ┌┴─────────┴┐ | ||
| │ Component │ ← @testing-library/react | ||
| │ (moderate) │ Screen-level render + interaction | ||
| ┌┴───────────┴┐ | ||
| │ Integration │ ← redux-saga-test-plan | ||
| │ (moderate) │ Saga + reducer end-to-end | ||
| ┌┴─────────────┴┐ | ||
| │ Unit │ ← Jest | ||
| │ (many, fast) │ Pure functions, reducers | ||
| └───────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Specify fence languages for the ASCII/code layout blocks.
Both fenced blocks are missing language identifiers (MD040). Mark them as text.
Proposed fix
-```
+```text
┌─────────┐
│ E2E │ ← Playwright + Electron (PR 2)
│ (few) │
┌┴─────────┴┐
│ Component │ ← `@testing-library/react`
│ (moderate) │ Screen-level render + interaction
┌┴───────────┴┐
│ Integration │ ← redux-saga-test-plan
│ (moderate) │ Saga + reducer end-to-end
┌┴─────────────┴┐
│ Unit │ ← Jest
│ (many, fast) │ Pure functions, reducers
└───────────────┘
```diff
-```
+```text
src/
├── __tests__/ # Tests, organized by layer
│ ├── utils/ # L1 — pure functions, selectors
│ ├── reducers/ # L1 — reducer three-contracts tests
│ ├── sagas/ # L2 — saga integration
│ ├── screens/ # L3 — screen-level component tests
│ └── components/ # L3 — reusable component tests
├── __mocks__/ # Manual mocks for npm packages
│ ├── `@hathor/`
│ ├── `@ledgerhq/`
│ ├── `@reown/`
│ ├── `@sentry/`
│ ├── `@walletconnect/`
│ └── unleash-proxy-client.js
├── test-utils/ # Shared helpers
│ ├── createTestStore.js
│ ├── renderWithProviders.js
│ ├── mockNavigation.js
│ ├── getInitialState.js
│ ├── ledgerTransportMock.js
│ └── index.js # Barrel re-exports
└── setupTests.js # Activates the centralized mocks
</details>
Also applies to: 57-80
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/testing-guide.md around lines 34 - 48, The two fenced code blocks (the
ASCII test pyramid diagram and the src/ tree) in docs/testing-guide.md are
missing language identifiers which triggers MD040; update both opening fences
from totext so the blocks are explicitly marked as plain text (apply the
same change to the other occurrence around lines 57-80); ensure only the opening
fence is changed to include the language identifier for the ASCII diagram and
the src/ tree blocks.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| "@sentry/browser": "7.99.0", | ||
| "@sentry/cli": "2.27.0", | ||
| "@testing-library/cypress": "10.0.2", | ||
| "@testing-library/jest-dom": "5.17.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of @testing-library/jest-dom and are there any security advisories for version 5.17.0?
💡 Result:
Latest stable version of @testing-library/jest-dom: 6.9.1 (released 2025-10-01) [1][2][3]. Security advisories for version 5.17.0: - There is no published security policy/advisory page for the project (GitHub shows “No security policy detected” and that no SECURITY.md file has been set up) [4]. - Snyk’s vulnerability page for @testing-library/jest-dom@5.17.0 reports “No direct vulnerabilities have been found for this package” in its vulnerability database (it also notes this excludes vulnerabilities in dependencies) [5]. So, based on the sources above, I did not find any direct security advisories for jest-dom 5.17.0; the repo itself does not appear to publish formal advisories via a security policy page [4], and automated scanning in Snyk shows no direct vulnerabilities for that specific version [5].
Citations:
- 1: https://github.com/testing-library/jest-dom/releases
- 2: https://www.npmjs.com/package/@testing-library/jest-dom
- 3: https://github.com/testing-library/jest-dom?tab=readme-ov-file
- 4: https://github.com/testing-library/jest-dom/security
- 5: https://security.snyk.io/package/npm/%40testing-library%2Fjest-dom/5.17.0
Update @testing-library/jest-dom to the latest stable release (and review for breaking changes)
@testing-library/jest-dom is pinned to 5.17.0, while the latest stable version is 6.9.1. No direct security advisories for 5.17.0 were found (Snyk reports no direct vulnerabilities; the project doesn’t publish a security policy/advisory page). Still, update for maintenance and run tests to catch any breaking changes from the major version bump.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 109, Update the dependency entry for
`@testing-library/jest-dom` in package.json from "5.17.0" to the latest stable
release (e.g., "6.9.1"), run npm/yarn install, then run the test suite and fix
any failing assertions caused by the major-version API changes; specifically
review usages of jest-dom matchers in your tests (imports and matcher names) and
update code to the v6 API where required.
Comment on lines
+37
to
+38
| middleware: (getDefaultMiddleware) => | ||
| getDefaultMiddleware({ serializableCheck: false }).concat(sagaMiddleware, thunk), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if production store configuration also duplicates thunk middleware
# Search for configureStore calls in production code to verify middleware setup
rg -n -A 10 "configureStore\(" --type js --type ts -g '!**/__tests__/**' -g '!**/*.test.*' -g '!**/test-utils/**'Repository: HathorNetwork/hathor-wallet
Length of output: 668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portions of the test store implementation
echo "---- src/test-utils/createTestStore.js (around middleware config) ----"
nl -ba src/test-utils/createTestStore.js | sed -n '1,120p'
# Show production store config and the middleware composition it uses
echo "---- src/store/index.js (full file or up to 200 lines) ----"
nl -ba src/store/index.js | sed -n '1,220p'
# Identify where "middlewares" is defined/imported in the production store
echo "---- Locate 'middlewares' usage/definition ----"
rg -n "middlewares" src/store/index.js src/store -S || true
# Check RTK version to align with getDefaultMiddleware behavior
echo "---- package.json dependencies (rtk/redux-thunk) ----"
cat package.json | rg -n "\"`@reduxjs/toolkit`\"|\"redux-thunk\"|\"react-redux\"" || trueRepository: HathorNetwork/hathor-wallet
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/test-utils/createTestStore.js (lines 1-120) ----"
cat -n src/test-utils/createTestStore.js | sed -n '1,160p'
echo "---- src/store/index.js (lines 1-220) ----"
cat -n src/store/index.js | sed -n '1,260p'
echo "---- Find definition of 'middlewares' in src/store/index.js ----"
rg -n "const middlewares|middlewares\s*=" src/store/index.js || true
echo "---- Find where sagaMiddleware/thunk are declared in src/store/index.js ----"
rg -n "sagaMiddleware|thunk" src/store/index.js || true
echo "---- Locate thunk import usage in src/test-utils/createTestStore.js ----"
rg -n "thunk|getDefaultMiddleware" src/test-utils/createTestStore.js
echo "---- package.json (RTK version / deps) ----"
cat package.json | rg -n "\"`@reduxjs/toolkit`\"|\"redux-thunk\"|\"react-redux\""Repository: HathorNetwork/hathor-wallet
Length of output: 4033
Remove redundant redux-thunk middleware from both test and production store configuration.
getDefaultMiddleware() in RTK 2.2.3 already includes thunk, but both stores add it again:
src/test-utils/createTestStore.jsline 38:getDefaultMiddleware(...).concat(sagaMiddleware, thunk)src/store/index.jslines 19-23 (withmiddlewares = [sagaMiddleware, thunk]at line 15):getDefaultMiddleware(...).concat(middlewares)
This duplication is consistent across test and production; it’s still redundant and should be cleaned up (e.g., drop the explicit thunk from the concat arrays).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test-utils/createTestStore.js` around lines 37 - 38, The Redux store
configuration is adding redux-thunk redundantly because getDefaultMiddleware()
already includes thunk; remove the explicit thunk addition: update
createTestStore.js where middleware is built by
getDefaultMiddleware(...).concat(sagaMiddleware, thunk) to drop the thunk so it
only concatenates sagaMiddleware, and likewise update the production store setup
where the middlewares array currently contains [sagaMiddleware, thunk] (and
where getDefaultMiddleware(...).concat(middlewares) is used) to remove thunk
from that array; keep sagaMiddleware and ensure only sagaMiddleware is appended
to getDefaultMiddleware so thunk remains provided by RTK's getDefaultMiddleware.
The previous lockfile (generated with npm 11.9) omitted optional peer deps as concrete node_modules entries. CI's npm 10.9 (Node 22) requires them present, causing the e2e job's `npm ci` to fail at lockfile validation. Regenerated via `npm install --package-lock-only` in node:22; the diff is exactly the three entries CI flagged as missing, all marked optional peer deps: - node_modules/react-scripts/node_modules/type-fest@4.41.0 - node_modules/ttag/node_modules/babel-plugin-macros@3.1.0 - node_modules/ttag/node_modules/resolve@1.22.12 No version bumps, no integrity changes for existing entries.
Three small fixes from the PR review, all in test infrastructure:
- src/__mocks__/@ledgerhq/hw-transport-node-hid.js: TransportNodeHid.listen
now returns { unsubscribe: jest.fn() } to mirror the real API contract
(the real listen() returns a Subscription handle). Defensive — no current
caller exercises this, but the mock should match real behavior.
- src/setupTests.js: the stubbed Redux store's subscribe() now returns an
unsubscribe function instead of undefined. Aligns with the Redux subscribe
contract so any consumer that imports the stub and follows the contract
doesn't crash.
- src/__tests__/sagas/modal.test.js: errorSpy.mockRestore() moved from
inside .then() to .finally(). The previous arrangement leaked the
console.error spy across tests in the file if expectSaga(...).silentRun()
ever rejected.
Verified full jest suite passes in node:22 (15/15 active suites, 75 tests,
0 failures).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #870 +/- ##
=========================================
- Coverage 9.18% 7.34% -1.85%
=========================================
Files 113 182 +69
Lines 5269 7991 +2722
Branches 703 1084 +381
=========================================
+ Hits 484 587 +103
- Misses 4117 6359 +2242
- Partials 668 1045 +377 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces the first three layers of the automated test suite for
hathor-wallet(desktop), scoped from RFC #114 (HathorNetwork/rfcs#114) where the multi-layer testing pyramid is designed and motivated. This is the desktop sibling of hathor-wallet-mobile#863, which lands the same three layers for the mobile wallet against the corresponding RFC #110.utils/selectors,utils/tokens,reducer.walletlifecycle.modalandwalletsagas viaredux-saga-test-plan.Welcome,WalletType, andSoftwareWalletWarningscreens via@testing-library/react+@testing-library/jest-dom.Layer 4 (E2E) is intentionally excluded from this PR — it is the largest and most contentious layer (framework choice, Electron build pipeline, CI cost) and will land separately so reviewers can evaluate Layers 1-3 on their own merits. See RFC #114 for the full rationale and the Layer 4 evaluation work.
What's in
@testing-library/jest-dom,redux-saga-test-plan. (@testing-library/react,@testing-library/user-event, and@testing-library/cypresswere already inpackage.json.)src/test-utils/{createTestStore,getInitialState,mockNavigation,renderWithProviders,ledgerTransportMock,index}shared across specs.src/setupTests.js: jest-dom matchers + global test setup (replaces ad-hoc per-file setup that previously lived inline).src/__mocks__/for desktop-specific dependencies whose real implementations don't work under jsdom:@hathor/hathor-rpc-handler,@hathor/wallet-lib,@ledgerhq/hw-transport-node-hid,@reown/walletkit,@sentry/electron,@walletconnect/core,@walletconnect/utils,unleash-proxy-client.ModalPin.test.js,ModalSendTx.test.js,CreateToken.test.js,atomicSwap.test.js— small touch-ups so the existing legacy specs run alongside the new ones.clearModalContextForTestingexported fromsrc/sagas/modal.js, purely for Layer 2 test isolation (parallels mobile'sclearLoadingLocksForTesting). Marked test-only by theForTestingsuffix convention and documented as such inline.src/utils/selectors.js— three small Redux selectors (selectIsOnline,selectSelectedToken,selectTokenByUid) created as the reference module for the selector-co-location pattern that follow-up feature-area PRs will use as they extract selector logic out ofmapStateToPropsblocks.AGENTS.mdandCLAUDE.mdat the repo root — concise repo conventions for any AI coding agent (cross-tool format, same shape as the mobile sibling)..claude/skills/writing-tests/SKILL.md— Claude Code skill that auto-loads on test work and encodes the conventions surfaced by repeated reviews so each new test PR doesn't re-fight the same nits.docs/testing-guide.md: rich human-readable reference covering the four-layer pyramid, per-PR test checklist, design principles, and future-proofing patterns..github/workflows/main.ymlso these tests actually gate PRs.What's NOT in
cypress/directory is untouched).clearModalContextForTestingtest-only export and the newsrc/utils/selectors.jsreference module.main.yml.Acceptance criteria
npm teston a fresh clone reports zero failures and Layer 1-3 deliverable suites are green.clearModalContextForTestingtest-only export insrc/sagas/modal.js, andsrc/utils/selectors.jsreference module (pure selectors, no callers yet — exists to anchor the pattern for follow-up PRs).AGENTS.md(cross-tool entry point),CLAUDE.md(Claude Code pointer),.claude/skills/writing-tests/SKILL.md(auto-loads on test work), anddocs/testing-guide.md(long-form reference). A new contributor — human or agent — can add a test that follows the repo's contracts and review conventions without prior session context.Security checklist
@testing-library/jest-dom,redux-saga-test-plan) are dev-only (devDependencies). Existing@testing-library/*packages were already inpackage.json.Design rationale
See RFC #114 for: layer definitions and scope boundaries, dependency choices (RTL for web vs Enzyme,
redux-saga-test-planvs manualgen.next()driving for sagas), file-organization conventions, the test-only export convention (ForTestingsuffix), the selector-co-location reference-module pattern, and the rationale for deferring Layer 4 (E2E) to a separate review cycle.Diff size breakdown
src/__tests__/)src/__mocks__/)src/test-utils/,src/setupTests.js)AGENTS.md,CLAUDE.md,.claude/skills/...,docs/testing-guide.md)package-lock.json+package.json.github/workflows/main.yml)src/sagas/modal.jstest-only export +src/utils/selectors.jsreference module)Summary by CodeRabbit
Tests
Documentation
Chores